-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor Copr build to focus on chroot #356
Conversation
6c436b8
to
6d12f17
Compare
@evgeni I am needing som ehelp if you have any thoughts on why this is happening. First look at this output:
That |
@evgeni Thanks for the fix! Now can I ask for a review too? :) |
obal/data/module_utils/obal.py
Outdated
for (macro, value) in macros.items(): | ||
command += ['--define', '"%s %s"' % (macro, value)] | ||
|
||
return subprocess.check_output(' '.join(command), universal_newlines=True, shell=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is copied code, but I wonder if shell=True
is really needed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without that we get:
File "/tmp/ansible_copr_build_payload_5hf5no4a/ansible_copr_build_payload.zip/ansible/module_utils/obal.py", line 27, in macro_lookup
File "/usr/lib64/python3.10/subprocess.py", line 421, in check_output
return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
File "/usr/lib64/python3.10/subprocess.py", line 503, in run
with Popen(*popenargs, **kwargs) as process:
File "/usr/lib64/python3.10/subprocess.py", line 971, in __init__
self._execute_child(args, executable, preexec_fn, close_fds,
File "/usr/lib64/python3.10/subprocess.py", line 1847, in _execute_child
raise child_exception_type(errno_num, err_msg, err_filename)
FileNotFoundError: [Errno 2] No such file or directory: 'rpmquery --queryformat %{name} --package /tmp/SRPMs/hello-2.10-1.src.rpm --undefine dist'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha. Yeah. Drop that join and pass a list, then it should work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets see what the tests say...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to drop the quotes (when constructing the define) now, as there is no shell anymore and thus no quoting is needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems to have done it.
Does this change mean that for a package that has multiple chroots defined, we now execute multiple Not blocking per se, but something that can come bite us at some point when we add EL9 (We already have this "issue" today with the non-tito koji builds, so it's strictly speaking not a regression [anymore]) |
We do execute multiple |
23091a5
to
d57b7c7
Compare
It doubles the build time when it builds el8 and then 9 |
I attempt to reduce this by not waiting for the builds to finish and instead watching the tasks, that is it goes:
|
Oh right right! |
d57b7c7
to
9383096
Compare
Updated with comments |
6502630
to
422105e
Compare
The Copr build functionality was on partially considering the chroot when looking at if a build exists. Further, there was a lot of copied code between release and scratch that is slimmed down here to allow the by chroot method to properly work.
422105e
to
4e71d45
Compare
The Copr build functionality was on partially considering the chroot when looking at if a build exists. Further, there was a lot of copied code between release and scratch that is slimmed down here to allow the by chroot method to properly work.
By and large, I needed to lean more into doing everything by chroot and this includes existence. So I've removed the dedicated check for existence and instead turned copr_build into a more idempotent module that checks existence and acts based upon this.